Skip to content

writers.writeNginxConfig: use crossplane to test the output#219247

Closed
RossComputerGuy wants to merge 1 commit intoNixOS:masterfrom
RossComputerGuy:feat/nginx-writer-test
Closed

writers.writeNginxConfig: use crossplane to test the output#219247
RossComputerGuy wants to merge 1 commit intoNixOS:masterfrom
RossComputerGuy:feat/nginx-writer-test

Conversation

@RossComputerGuy
Copy link
Member

@RossComputerGuy RossComputerGuy commented Mar 2, 2023 •
edited
Loading

Description of changes

Modifies writers.writeNginxConfig to use nginx -t as gixy doesn't check everything.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 2, 2023
lilyinstarlight
lilyinstarlight reviewed Mar 2, 2023
View reviewed changes
@RossComputerGuy RossComputerGuy force-pushed the feat/nginx-writer-test branch from 38f55d1 to 538f036 Compare March 3, 2023 00:51
@lilyinstarlight
Copy link
Member

lilyinstarlight commented Mar 3, 2023

@ofborg build nixosTests.nginx nixosTests.nginx-auth nixosTests.nginx-etag nixosTests.nginx-globalredirect nixosTests.nginx-http3 nixosTests.nginx-modsecurity nixosTests.nginx-njs nixosTests.nginx-pubhtml nixosTests.nginx-sandbox nixosTests.nginx-sso nixosTests.nginx-variants

@Lassulus
Copy link
Member

Lassulus commented Mar 3, 2023 

error: builder for '/nix/store/5zjmqwpgc047byc9zmpvrkgi960clhm6-nginx.conf.drv' failed with exit code 5;
       last 10 log lines:
       >
       > ==================== Summary ===================
       > Total issues:
       >     Unspecified: 0
       >     Low: 0
       >     Medium: 0
       >     High: 0
       >
       > invalid number of arguments in "proxy_pass" directive in /nix/store/h007vhip0yxyp05mh34grsakz6y8dd2m-nginx.conf:44
       > "server" directive is not allowed here in /nix/store/h007vhip0yxyp05mh34grsakz6y8dd2m-nginx.conf:47
``` when I try to run the nginx test.

@ajs124
Copy link
Member

ajs124 commented Mar 3, 2023

Can you also adjust the title and commit message.

I thought this was probably something similar to #205561 (and #207532), which was reverted.

@RossComputerGuy RossComputerGuy changed the title writers.writeNginxConfig: use nginx to test the output writers.writeNginxConfig: use crossplane to test the output Mar 3, 2023
@RossComputerGuy RossComputerGuy force-pushed the feat/nginx-writer-test branch from 538f036 to eb14d7a Compare March 3, 2023 21:31
@RossComputerGuy
Copy link
Member Author

RossComputerGuy commented Mar 3, 2023

Can you also adjust the title and commit message.

Done

invalid number of arguments in "proxy_pass" directive in /nix/store/h007vhip0yxyp05mh34grsakz6y8dd2m-nginx.conf:44

I'll take a look at this, where's the test defined?

@RossComputerGuy
Copy link
Member Author

RossComputerGuy commented Mar 3, 2023

I see that the line where the error occurs at is:

proxy_pass ;;;;

I looked at the documentation for the proxy module's proxy_pass command and it looks like it requires an argument so we just need to change it.

@lilyinstarlight
Copy link
Member

lilyinstarlight commented Mar 8, 2023 •
edited
Loading

I see that the line where the error occurs at is:

proxy_pass ;;;;

I looked at the documentation for the proxy module's proxy_pass command and it looks like it requires an argument so we just need to change it.

It looks like that is a test that is intentionally supposed to fail at runtime (in specialisation.reloadWithErrorsSystem.configuration)

Perhaps we just need to add an escape hatch to turn off syntax checking, or we could make that test use something that isn't a syntax error but still fails nginx -t (e.g. including a non-existent file)

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@RossComputerGuy
Copy link
Member Author

RossComputerGuy commented Apr 9, 2024

I don't care about this anymore

@RossComputerGuy RossComputerGuy deleted the feat/nginx-writer-test branch April 9, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lassulus Lassulus Awaiting requested review from Lassulus

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

1 more reviewer

@lilyinstarlight lilyinstarlight lilyinstarlight left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@RossComputerGuy @lilyinstarlight @Lassulus @ajs124 @wegank